-
Notifications
You must be signed in to change notification settings - Fork 0
Ap 226: Update Framework's Dockerfile to use the most recent Node LTS #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
awilfox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a refreshingly small change to be able to upgrade this far up - I'm quite happy to see that!
I do have a few questions about the changes before approving it.
Dockerfile
Outdated
| curl \ | ||
| python3 | ||
|
|
||
| RUN apt-mark manual python3.13-minimal \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to hardcode the version? (Is that what the new Node depends on instead of just '3'?)
And is it intentional that we aren't auto removing python3(.13) any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple Python versions installed. I kept python3.13-minimal, which works correctly. Autoremoving python3 triggers removal of Node.js dependencies, which causes failures in the production stage when yarn install runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aren't auto-removing Python, then IMO we don't really need to mark anything manual.
But I think it'd be better to mark the real dependencies as dependencies. This isn't just a size concern; having extraneous dependencies can allow attackers "chain" attacks (i.e. if they can execute limited code, but it includes Python code, and there's a further CVE in Python, they can use that) so it is a security best-practice to only include what we need.
If we keep python3.13-minimal and auto remove python3, what other dependencies does Node need? If it's just a few, we might be able to figure out something out.
If you like, I can toy around with this on my computer after I finish my work with AP-270.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Node runtime does not need Python, only the yarn installation needs Python. Let me take a look again too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does yarn need Python anymore if it's being installed with corepack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the yarn install in production stage still need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When python3 is removed, node.js becomes unavailable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Node.js v24.13.0 (NodeSource/Debian slim), Python3 is a required dependency, which is why removing Python3 also remove Node.js. This differs from Node.js 16, where Python3 was not required. Considering the security concern raised by Anna, we should keep Python 3 installed here, so I removes the code to set 'python3.13-minimal' as manual one.
Also I would suggest to make a follow-up decision on how we want to manage those dependencies in long term:
Option A: switch to the official Node.js binary installation to avoid apt related dependencies like Python.
Option B: moving Node.js/Yarn installation from base stage to development or a build stage and copy related libraries into production stage. My testing show Node.js and Yarn still existed in production image even when yarn install is removed from production stage, we may revisit the need of yarn install in the production stage.
anarchivist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comments/questions are basically the same as @awilfox's - if we can resolve those, then i think this might be ready to go.
Update Node and Yarn to the latest:
Node: v24.13.0
Yarn: 1.22.22